Skip to content

Added support for number type attributes#68

Open
dw2 wants to merge 4 commits into
AmpersandJS:masterfrom
dw2:master
Open

Added support for number type attributes#68
dw2 wants to merge 4 commits into
AmpersandJS:masterfrom
dw2:master

Conversation

@dw2

@dw2 dw2 commented Feb 9, 2016

Copy link
Copy Markdown

This change will allow model attributes with type 'number' to work with the select view. As an added bonus, this will also allow for mixed option sets (i.e. [[0,'First'], 2, 3, [4,'Fourth'], 5, ...]).

@dw2

dw2 commented Feb 9, 2016

Copy link
Copy Markdown
Author

This resolves issue #66

@cdaringe

cdaringe commented Feb 9, 2016

Copy link
Copy Markdown
Member

thanks @dw2! i now see what you're sayin. :)

wraithgar and I had a long thread with another user about permitting the use of mixed types. at current time, we are hesistant to support multiple option types in the options set, per discussion here. also, mixing of some types are just plain incompatible. however, we definitely do want to support numbers only. can't believe this has never come up before!

very grateful for your PR. for the time being, would you be kind enough to:

  1. add a test? they should be relatively straightforward. to imitate. see the test folder
  2. find the matching option within common types? if you want to use .find and be more functional about it, thats ok too. :)

@dw2

dw2 commented Feb 9, 2016

Copy link
Copy Markdown
Author
  1. Sure, I can add a test. I'll try to use the other tests as an example.
  2. Can you explain what you mean by "common types" and using ".find"? Do you mean adding checks for other attribute types, like booleans and dates? Something else?

@cdaringe

cdaringe commented Feb 9, 2016

Copy link
Copy Markdown
Member

Can you explain what you mean by "common types"

apologies for the lack of clarity. "find the matching option within common types?" should have read "adjust the code to find the matching option assuming that the options in this.options are homogeneous. e.g. 'remove support for mixed option sets'."

and using ".find"?

if you don't want to use the status quo for loop, you're welcome to find the matching option using this.options.find(function(option) { ... }). just conveying no need to maintain the status quo if you didnt care to.

@dw2

dw2 commented Feb 9, 2016

Copy link
Copy Markdown
Author

I removed support for mixed option types and added a test for numbers when using [[number','label'], ... option types. However I am unable to run the test locally because I cannot get zuul installed properly to run the test suite.

@wraithgar

Copy link
Copy Markdown
Contributor

checked out branch as of 82ad163 and the tests pass locally for me so let's not let that be a blocker here, I'm currently trying to help doug get zuul running via other messaging channels.

@dw2

dw2 commented Feb 9, 2016

Copy link
Copy Markdown
Author

@wraithgar helped me out via Twitter DM. I can run tests locally now, but am still having separate issues with zuul. I'm taking this as a win. Thank you @wraithgar and @cdaringe for the help and feedback. Let me know if you need anything else for this PR. I hope to contribute more in the future.

@dw2

dw2 commented Feb 9, 2016

Copy link
Copy Markdown
Author

Also, thank you @jweakley for helping me troubleshoot NPM issues.

Comment thread ampersand-select-view.js
}
}
} else {
throw new Error('no options provided');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like and support this, we tend to want to throw exceptions when devs get themselves into places that would give unexpected results. This will mean a major release though because it would not have thrown in this case before.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think I should remove this for now and do a separate PR for it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! Leave it. Versions are cheap I'm just making sure we see that so we don't miss it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants